First draft of CLI docker integration #7
Conversation
src/internal/docker/cli.go
Outdated
| yey "github.com/silphid/yey/src/internal" | ||
| ) | ||
|
|
||
| type CLI struct{} | ||
|
|
||
| func (c CLI) Start(ct yey.Context, imageTag, containerName string) error { | ||
| return fmt.Errorf("not implemented") | ||
| func NewCLI() CLI { |
There was a problem hiding this comment.
Question I have been wondering. Do we need an empty type? Do we need a yey.Docker interface that this CLI type needs to implement?
Basically the question becomes, are we injecting the this implementation somewhere? Are we planning on mocking the results of CLI[Method] ?
Would these tests add value?
I am not sure of this, but I wouldn't mind simply having the docker package export functions that wrap os/exec calls,
and having integration tests against actual docker versions as a first pass.
Food for thought.
There was a problem hiding this comment.
effectivement, je vois pas le besoin du empty struct ou interface à court terme, juste pour les tests éventuellement, mais on peut l'enlever pour l'instant
src/internal/docker/cli.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| args := []string{"run"} |
There was a problem hiding this comment.
I think it might be clearer if instead of modified arg slices, we had smaller docker building blocks, like a run function and a start function. that way it could simply be:
if running {
return containerExec(containerName, ...)
}
return containerRun(containerName, ...)
src/internal/docker/cli.go
Outdated
| // Common args | ||
| args := []string{"-it", "--env LS_COLORS", "--env TERM", "--env TERM_COLOR", "--env TERM_PROGRAM"} | ||
| if !isExec { | ||
| args = append(args, "--rm", "--name", containerName) |
There was a problem hiding this comment.
TODO Discuss the philosophy of --rm
There was a problem hiding this comment.
Y'a tellement de flags docker possibles et j'aimerais mieux pas tous les hardcoder si possible (genre "--network=host" qui est très pratique, mais pas nécessairement ce que tout le monde veut). Alors, je pensais pouvoir configurer des flags explicitement dans le contexte. Comme ça, tout le monde pourrait contrôler ces flags qui sont plus personnels.
| } | ||
|
|
||
| // Context env vars | ||
| for name, value := range yeyCtx.Env { |
There was a problem hiding this comment.
Isn't the environment only needed to be passed at run? Shouldn't exec into a container that was run with an env keep the env that was set previously?
There was a problem hiding this comment.
c'est une question que je me demandais, à tester quand on aura un moment
There was a problem hiding this comment.
je pense qu'en faisaint un split entre exec et run tu vas ne meme pas avoir besoin de cette fonction apres, car tout c'est args la sont seulement vraiment important quand tu fais run. Et donc tu n'auras pas besoin d'une function a part comme celle ci.
There was a problem hiding this comment.
je viens de vérifier et les env vars sont persistants across docker run, exec et start.
No description provided.